Repository navigation
Reduce memory used by repeated buffered File writes - #32
Conversation
284km
left a comment
284km
left a comment
There was a problem hiding this comment.
Reviewed c3e88ed against 23ce853. No actionable findings in this change.
The buffer now reuses the existing String chunk queue, and the emptiness check avoids materializing that queue on every append. I also checked the surrounding flush/reset, GC rooting, pushback, encoding, and File#dup paths.
Validation:
- Fresh O2 builds of base and head with the same Mere
991f299bcompiler and mgz revision. - The new buffered-write regression matched Ruby 4.0.9 on both revisions, with hash indexing enabled/disabled and with normal GC or
MERE_RUBY_GC_STRESS=100(eight runs). - Additional probes for source-string mutation after write, empty writes, pushback, reopen, and UTF-16LE writes matched Ruby on both revisions with indexing enabled/disabled under GC stress (four runs).
- Two benchmark runs per revision, using 16,384 writes: peak process RSS fell from 274.4–281.9 MiB to 215.9–220.6 MiB. All benchmark stdout checks matched Ruby. Measured workload times were 4.61–5.42 s before and 3.76–3.84 s after; these are local measurements, not an end-to-end CI result.
- The CI corpus job for this exact head reports 509/509 programs matching Ruby; the Linux build also passed. I did not repeat the full corpus or the FileUtils measurement locally. Some broader test shards are still pending.
git diff --checkpassed.
284km
left a comment
284km
left a comment
There was a problem hiding this comment.
Follow-up review of c3e88ed: the source is unchanged from my previous review, and there are no outstanding findings from that review. The PR is now merged as 2004502; all 29 CI jobs for the reviewed head passed.
The buffered-write regression also matched Ruby 4.0.9 in both index modes during this re-review of #33's rebased head, which includes this implementation. No new local build or repeated benchmark was needed for this unchanged PR.
| let _ = map_set iv9 (k9 ++ "@__f_buf") (mkstr (cur9 ++ piece)) in | ||
| let _ = (if empty9 then map_set iv9 (k9 ++ "@__f_wpos") (VInt pos9) else ()) in | ||
| let _ = (match buf9 with | ||
| | VStr h9 -> str_push h9 piece |
There was a problem hiding this comment.
Positive: The existing queued String append is preserved in the merged code. Its buffered-write regression also passes alongside the rebased positioned-read implementation in #33 with indexing ON and OFF.
Repeated small File writes currently copy the entire accumulated buffer and allocate a new String handle on each append. FileUtils' large-file fixture consequently approaches the 2 GiB measurement limit despite writing less than 1 MiB. Append to the existing String chunk queue, retaining the existing flush, logical-position, pushback, and encoding behavior.
A deterministic local run of
fileutils/test_fileutils.rbcompleted all 117 tests with 304 MiB peak RSS, compared with 1,549–1,793 MiB in two baseline runs. The same five existing failures remain. CI limits and test inputs are unchanged.Validation:
The Linux regression passed. The completed CI shard ran all 117 FileUtils tests with 458,336 KiB peak process-family RSS and the same five existing failures; Queue completed all 41 tests successfully.
CI for this head passed all 29 jobs. All 758 CRuby test files were collected from 8 shards with no missing files or infrastructure errors. This confirms complete measurement, not full Ruby conformance. The local integration of #32–#36 with main
7ac8c4calso passed all 514 corpus programs with hash indexing ON and OFF; TCP passed 22 tests, Queue passed 41, and FileUtils and Unicode retained only their baseline failures.